Skip to content

ARROW-5775: [C++] Fix thread-unsafe cached data #4791

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
pitrou wants to merge 2 commits into from
Closed

ARROW-5775: [C++] Fix thread-unsafe cached data #4791

pitrou wants to merge 2 commits into from

Conversation

@pitrou
Copy link
Member

@pitrou pitrou commented Jul 3, 2019

No description provided.

@pitrou
Copy link
Member Author

pitrou commented Jul 3, 2019 •
edited
Loading

Hmm... so it looks like gcc 4.8.2 in manylinux1 may not have these APis:
https://en.cppreference.com/w/cpp/memory/shared_ptr/atomic

https://stackoverflow.com/questions/34205880/why-does-atomic-load-with-a-shared-ptr-not-compile-with-gcc-4-9

@pitrou
Copy link
Member Author

pitrou commented Jul 3, 2019

Perhaps we can workaround that by emulating atomic_load and atomic_store on that old gcc using a global lock.

@pitrou
Copy link
Member Author

pitrou commented Jul 3, 2019

Note that manylinux2010 works fine... one more reason to switch.

@pitrou pitrou force-pushed the ARROW-5775-boxed-fields branch 2 times, most recently from 028b5d8 to 52a82a9 Compare July 3, 2019 13:37
@pitrou
Copy link
Member Author

pitrou commented Jul 3, 2019

Travis-CI: https://travis-ci.org/pitrou/arrow/builds/553762594

@pitrou
Copy link
Member Author

pitrou commented Jul 3, 2019

AppVeyor: https://ci.appveyor.com/project/pitrou/arrow/builds/25722036

@codecov-io
Copy link

codecov-io commented Jul 3, 2019

Codecov Report

Merging #4791 into master will increase coverage by 1.64%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4791      +/-   ##
==========================================
+ Coverage   87.43%   89.08%   +1.64%     
==========================================
  Files         996      720     -276     
  Lines      139677   100040   -39637     
  Branches     1418        0    -1418     
==========================================
- Hits       122124    89117   -33007     
+ Misses      17191    10923    -6268     
+ Partials      362        0     -362
Impacted Files Coverage Δ
cpp/src/arrow/array.h 98.29% <ø> (ø) ⬆️
cpp/src/arrow/util/atomic_shared_ptr.h 100% <100%> (ø)
cpp/src/arrow/record_batch.cc 87.69% <100%> (+0.09%) ⬆️
cpp/src/arrow/python/deserialize.cc 92.41% <100%> (ø) ⬆️
cpp/src/arrow/array.cc 80.72% <100%> (ø) ⬆️
go/arrow/ipc/writer.go
js/src/util/fn.ts
go/arrow/memory/memory_avx2_amd64.go
rust/datafusion/src/execution/filter.rs
rust/arrow/src/csv/writer.rs
... and 275 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a9a82ec...52a82a9. Read the comment docs.

@kszucs kszucs force-pushed the ARROW-5775-boxed-fields branch from 52a82a9 to 24f6dcd Compare July 4, 2019 07:38
kszucs
kszucs reviewed Jul 4, 2019
View reviewed changes
@pitrou pitrou force-pushed the ARROW-5775-boxed-fields branch from 24f6dcd to 5df85c4 Compare July 4, 2019 11:36
pitrou
pitrou commented Jul 4, 2019
View reviewed changes
cpp/CMakeLists.txt Outdated
Copy link
Member Author

@pitrou pitrou Jul 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kou Is that ok for you?

Copy link
Member

@xhochy xhochy Jul 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to simply go with #4801

kszucs
kszucs approved these changes Jul 4, 2019
View reviewed changes
Copy link
Member

@kszucs kszucs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pitrou needs a rebase

@pitrou pitrou force-pushed the ARROW-5775-boxed-fields branch from 5df85c4 to 9c2a333 Compare July 4, 2019 14:38
@pitrou
Copy link
Member Author

pitrou commented Jul 4, 2019

Done.

@kszucs
Copy link
Member

kszucs commented Jul 5, 2019

Appveyor R failure is unrelated.

@kszucs kszucs closed this in 0028b2b Jul 5, 2019
@pitrou pitrou deleted the ARROW-5775-boxed-fields branch July 5, 2019 08:55
wesm pushed a commit that referenced this pull request Jul 13, 2019
@pitrou @wesm
ARROW-5775: [C++] Fix thread-unsafe cached data
fa3f132 
Author: Antoine Pitrou <[email protected]>

Closes #4791 from pitrou/ARROW-5775-boxed-fields and squashes the following commits:

9c2a333 <Antoine Pitrou> Add "inline"
6fe0b86 <Antoine Pitrou> ARROW-5775:  Fix thread-unsafe cached data
@asfimport asfimport mentioned this pull request Aug 1, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@xhochy xhochy xhochy left review comments

@kszucs kszucs kszucs approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@pitrou @codecov-io @kszucs @xhochy